Implement HPX Future-Sender Bridge (P2300 interoperability) - #7256
Conversation
Up to standards ✅🟢 Issues
|
|
Can one of the admins verify this patch? |
|
@shivansh023023 What's the difference to the existing |
|
Thank you for pointing this out, @hkaiser I wasn't aware of the existing I've now reviewed those headers. Given that the core functionality already
I'd appreciate your guidance on which direction is most useful. I'm |
If that improves the integration, sure, please do that.
If tests are missing this is also a nice addition.
That's what I was asking. What would this PR add on top of what's already in place? |
Let's go with what you suggest. |
|
@shivansh023023 Do you still plan to work on this PR? |
yes , i am planning to work on this after #7260 |
FWIW, #7260 has been merged now. |
60ba99e to
f72c0da
Compare
|
@shivansh023023 FWIW, you seem to have missed adding a file to this PR. |
Up to standards ✅🟢 Issues
|
Added the file |
|
@shivansh023023 things don't compile, please have a look. |
|
@shivansh023023 ping? |
Sir , my end semester exams are going on , i'll surely look into the issue after 3-4 days |
ee1f291 to
0dae039
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds bidirectional conversion between HPX futures and P2300-style senders, registers and tests the adapters, adds a future-sender benchmark, and updates benchmark comments and header-list ordering. ChangesFuture-Sender Bridge
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant HPXFuture
participant FutureSender
participant Receiver
HPXFuture->>FutureSender: connect(receiver)
FutureSender->>HPXFuture: register continuation
HPXFuture-->>FutureSender: value or exception
FutureSender->>Receiver: set_value or set_error
sequenceDiagram
participant Sender
participant AsFuture
participant Promise
participant HPXFuture
AsFuture->>Sender: connect receiver
AsFuture->>Sender: start operation
Sender->>Promise: set value, error, or stopped
Promise-->>HPXFuture: make future ready
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
libs/core/futures/include/hpx/futures/future_sender.hpp (1)
21-27: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a direct include for
try_catch_exception_ptr.
hpx::detail::try_catch_exception_ptris used instart_helper(Lines 82, 89, 153, 156) but this header only includes config/future/execution_base and relies on a transitive include.sender_future.hppcorrectly pulls in<hpx/modules/errors.hpp>; add it here too to avoid a fragile dependency.♻️ Suggested include
`#include` <hpx/config.hpp> `#include` <hpx/futures/future.hpp> +#include <hpx/modules/errors.hpp> `#include` <hpx/modules/execution_base.hpp>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libs/core/futures/include/hpx/futures/future_sender.hpp` around lines 21 - 27, The header for future_sender uses hpx::detail::try_catch_exception_ptr in start_helper but does not include the header that declares it, relying on a transitive dependency. Update future_sender.hpp to add the direct include for hpx/modules/errors.hpp alongside the existing HPX includes so start_helper is self-contained and no longer depends on indirect includes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@libs/core/futures/include/hpx/futures/future_sender.hpp`:
- Around line 11-17: The usage example in future_sender.hpp refers to
hpx::execution::experimental::as_sender, but this header only exposes
future_sender<T>. Update the doc block to match the actual API used here, or add
the missing as_sender factory if that is intended; keep the example consistent
with future_sender and the stdexec usage shown.
---
Nitpick comments:
In `@libs/core/futures/include/hpx/futures/future_sender.hpp`:
- Around line 21-27: The header for future_sender uses
hpx::detail::try_catch_exception_ptr in start_helper but does not include the
header that declares it, relying on a transitive dependency. Update
future_sender.hpp to add the direct include for hpx/modules/errors.hpp alongside
the existing HPX includes so start_helper is self-contained and no longer
depends on indirect includes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a41a19aa-fe9b-40b7-b692-72db2cb272cb
📒 Files selected for processing (6)
libs/core/execution/tests/performance/benchmark_continues_on.cpplibs/core/futures/CMakeLists.txtlibs/core/futures/include/hpx/futures/future_sender.hpplibs/core/futures/include/hpx/futures/sender_future.hpplibs/core/futures/tests/unit/CMakeLists.txtlibs/core/futures/tests/unit/future_sender_test.cpp
- Bridge hpx::future<T> with P2300 senders. - Resolve header isolation test failures by using targeted futures includes. Signed-off-by: Shivansh Singh <singhshivansh023@gmail.com>
Signed-off-by: Shivansh Singh <singhshivansh023@gmail.com>
- Replaced deep internal module headers with <hpx/modules/futures.hpp> in tests to comply with C++20 modules constraints. - Removed non-ASCII characters (em-dashes, arrows, section symbols) across source files and tests to resolve hpxinspect violations. Signed-off-by: Shivansh Singh <singhshivansh023@gmail.com>
…re_sender_test Signed-off-by: Shivansh Singh <singhshivansh023@gmail.com>
Signed-off-by: Shivansh Singh <singhshivansh023@gmail.com>
Signed-off-by: Shivansh Singh <singhshivansh023@gmail.com>
Up to standards ✅🟢 Issues
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
libs/core/execution/tests/unit/future_sender_test.cpp (1)
122-141: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGood addition for
shared_futurecopyability, but consider coveringvoidresult types too.This test exercises
shared_future<int>, but the PR history notes CI failures were specifically caused by incompatible completion signatures forfuture<void>andshared_future<int>. Adding afuture<void>/shared_future<void>round-trip test here would directly guard against regressing that exact fix.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libs/core/execution/tests/unit/future_sender_test.cpp` around lines 122 - 141, Add a void-result round-trip test alongside test_shared_future_copyable_semantics, covering both future<void> and shared_future<void> through as_sender and sync_wait. Verify the sender pipeline completes successfully without requiring a value, directly guarding compatible completion signatures while preserving the existing int copyability assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@libs/core/execution/tests/unit/future_sender_test.cpp`:
- Around line 122-141: Add a void-result round-trip test alongside
test_shared_future_copyable_semantics, covering both future<void> and
shared_future<void> through as_sender and sync_wait. Verify the sender pipeline
completes successfully without requiring a value, directly guarding compatible
completion signatures while preserving the existing int copyability assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 658b90a9-a43f-4d9f-a1bd-e7273004e190
📒 Files selected for processing (3)
libs/core/execution/include/hpx/execution/algorithms/as_sender.hpplibs/core/execution/include/hpx/execution/algorithms/future_sender.hpplibs/core/execution/tests/unit/future_sender_test.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/core/execution/include/hpx/execution/algorithms/as_sender.hpp
Signed-off-by: guptapratykshh <pratykshgupta9999@gmail.com>
Signed-off-by: guptapratykshh <pratykshgupta9999@gmail.com>
Signed-off-by: guptapratykshh <pratykshgupta9999@gmail.com>
Signed-off-by: guptapratykshh <pratykshgupta9999@gmail.com>
…-opt Feat/as sender continues on opt
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/core/execution/include/hpx/execution/algorithms/future_sender.hpp (1)
35-49: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDrop
get_completion_scheduler<set_stopped_t>from the env
future_sender_with_scheduler::envadvertises a stopped completion scheduler, but these senders only complete withset_value_tandset_error_t. That makes downstream sender composition ill-formed. Restrict the query toset_value_tunless the sender can actually complete withset_stopped_t.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libs/core/execution/include/hpx/execution/algorithms/future_sender.hpp` around lines 35 - 49, Update future_sender_with_scheduler::env so its get_completion_scheduler query no longer advertises set_stopped_t; restrict the environment’s completion-scheduler support to set_value_t, matching future_sender_completion_signatures, while preserving set_error_t handling.
🧹 Nitpick comments (1)
libs/core/execution/include/hpx/execution/algorithms/as_sender.hpp (1)
26-121: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the obsolete sender wrappers
as_sender_tnow constructsdetail::future_sender/detail::future_sender_with_scheduler, andas_sender_sender_with_schedulerstill routes through the legacyas_sender_operation_state. Thesedetailtypes are only referenced here, so the old wrapper can go with the other obsolete implementation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libs/core/execution/include/hpx/execution/algorithms/as_sender.hpp` around lines 26 - 121, Remove the obsolete as_sender_operation_state class and its related implementation from this header, since as_sender_t now uses detail::future_sender and detail::future_sender_with_scheduler while as_sender_sender_with_scheduler should no longer depend on the legacy state. Clean up any now-unused declarations or references tied exclusively to this wrapper.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@libs/core/execution/include/hpx/execution/algorithms/future_sender.hpp`:
- Around line 35-49: Update future_sender_with_scheduler::env so its
get_completion_scheduler query no longer advertises set_stopped_t; restrict the
environment’s completion-scheduler support to set_value_t, matching
future_sender_completion_signatures, while preserving set_error_t handling.
---
Nitpick comments:
In `@libs/core/execution/include/hpx/execution/algorithms/as_sender.hpp`:
- Around line 26-121: Remove the obsolete as_sender_operation_state class and
its related implementation from this header, since as_sender_t now uses
detail::future_sender and detail::future_sender_with_scheduler while
as_sender_sender_with_scheduler should no longer depend on the legacy state.
Clean up any now-unused declarations or references tied exclusively to this
wrapper.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8c8b75bb-2dc3-4d52-a2c6-a2664ffea3d1
📒 Files selected for processing (5)
libs/core/execution/include/hpx/execution/algorithms/as_sender.hpplibs/core/execution/include/hpx/execution/algorithms/future_sender.hpplibs/core/execution/tests/performance/CMakeLists.txtlibs/core/execution/tests/performance/future_sender_bridge.cpplibs/core/execution/tests/unit/future_sender_test.cpp
|
@shivansh023023 please resolve the merge conflict and pay attention to the coderabbit comments. Those seem to be legit. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
libs/core/execution/tests/unit/future_sender_test.cpp (1)
185-200: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueVerify copyability by asserting or explicitly copying the sender.
The test's stated goal is to verify that the
shared_futurepath remains copyable. However, the current code movessndintosync_waitwithout ever evaluating its copy semantics, which leaves the copyability unverified.Consider adding a compile-time assertion and explicitly copying the sender to validate its copyability.
♻️ Proposed refactor to test copyability
void test_as_sender_with_scheduler_shared_future() { ex::thread_pool_scheduler sched{}; hpx::shared_future<int> f = hpx::make_ready_future(13); auto snd = ex::as_sender(f, sched); + static_assert(std::is_copy_constructible_v<decltype(snd)>); + auto snd_copy = snd; + auto result = - tt::sync_wait(std::move(snd) | ex::then([](int x) { return x + 1; })); + tt::sync_wait(std::move(snd_copy) | ex::then([](int x) { return x + 1; })); HPX_TEST(result.has_value()); HPX_TEST_EQ(std::get<0>(*result), 14); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libs/core/execution/tests/unit/future_sender_test.cpp` around lines 185 - 200, Update test_as_sender_with_scheduler_shared_future to verify the sender’s copyability explicitly: add a compile-time copyability assertion for the sender type and create a copied sender before invoking sync_wait, while preserving the existing result validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@libs/core/execution/tests/unit/future_sender_test.cpp`:
- Around line 185-200: Update test_as_sender_with_scheduler_shared_future to
verify the sender’s copyability explicitly: add a compile-time copyability
assertion for the sender type and create a copied sender before invoking
sync_wait, while preserving the existing result validation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4b61f3b4-761b-405c-af93-d9828f72c0a2
📒 Files selected for processing (5)
libs/core/execution/include/hpx/execution/algorithms/as_sender.hpplibs/core/execution/include/hpx/execution/algorithms/future_sender.hpplibs/core/execution/tests/performance/CMakeLists.txtlibs/core/execution/tests/performance/future_sender_bridge.cpplibs/core/execution/tests/unit/future_sender_test.cpp
🚧 Files skipped from review as they are similar to previous changes (3)
- libs/core/execution/tests/performance/CMakeLists.txt
- libs/core/execution/include/hpx/execution/algorithms/as_sender.hpp
- libs/core/execution/include/hpx/execution/algorithms/future_sender.hpp
|
@shivansh023023, I checked the merge conflict on #7256. It is only in libs/core/execution/tests/performance/CMakeLists.txt, because master added future_boundary and this PR added future_sender_bridge on the same benchmark list line |
Signed-off-by: Shivansh Singh <singhshivansh023@gmail.com>
|
@hkaiser , @guptapratykshh the conflicts have been resolved and the coderabbit suggestion is implemented too |
There is still one minor issue: https://cdash.rostam.cct.lsu.edu/viewBuildError.php?buildid=65628 |
Signed-off-by: guptapratykshh <pratykshgupta9999@gmail.com>
execution: fix future sender benchmark comment
|
@hkaiser lmk how things look to you |
https://cdash.rostam.cct.lsu.edu/viewBuildError.php?buildid=65701 |
Signed-off-by: guptapratykshh <pratykshgupta9999@gmail.com>
execution: avoid deprecated volatile compound assignment
|
@shivansh023023 Sorry for the delay. Great work. |
Implement
hpx::future↔ P2300 Sender Interoperability BridgeProposed Changes
libs/core/futures/include/hpx/futures/future_sender.hpp: implementsfuture_sender<T>, a P2300-compliant sender that wrapshpx::future<T>,exposing
as_sender(hpx::future<T>&&)as the public API entry pointlibs/core/futures/include/hpx/futures/sender_future.hpp: implementsas_future(sender)which converts any P2300 sender into anhpx::future<T>using an internal
sender_future_receiverthat bridges the two completionchannels
libs/core/futures/tests/unit/future_sender_test.cpp: unit testscovering future→sender conversion, sender→future conversion, error
propagation through the error channel, and move-only semantics verification
libs/core/futures/tests/unit/CMakeLists.txtfollowing the existing alphabetical ordering convention
Any background context you want to provide?
HPX currently has two async worlds that cannot communicate natively:
hpx::async()returnshpx::future<T>(HPX's own type)stdexeclibraryThis PR builds the translator layer between them, so that existing HPX
code producing futures can be seamlessly composed into modern P2300 pipelines,
and vice versa:
This is a foundational step toward the broader goal of re-implementing the
executor API on top of the sender/receiver infrastructure (issue #5219), as
it allows
hpx::async,hpx::dataflow, and other future-returning facilitiesto be gradually migrated into P2300 pipelines without breaking existing user
code.
Implementation follows all established HPX bridge conventions:
sender_concept/receiver_concepttags forstdexecconcept compliancetag_invokecustomization points (no virtual dispatch)HPX_CXX_CORE_EXPORTannotations for C++20 Module BMI visibilityfuturesandexecutorsmodulesChecklist